-
-
Notifications
You must be signed in to change notification settings - Fork 21
GH-1153 Rework delay system with per-entry Instant TTL #1184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the delay system to use per-entry TTLs, which is a significant improvement for avoiding cache-level conflicts. The introduction of DefaultDelay and ExplicitDelay interfaces, along with a factory, creates a much cleaner and safer API. My review highlights a critical issue where expireAfterWrite was unintentionally reintroduced in GuavaDefaultDelay, undermining the primary goal of this refactoring. I have also included some suggestions to improve the API design by reducing code duplication and better aligning the interface documentation with its behavior, which would enhance flexibility and maintainability.
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDefaultDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/DefaultDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/DefaultDelay.java
Outdated
Show resolved
Hide resolved
P1otrulla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job i like it but i'm thinking about overenginering at this case - maybe Im wrong
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request provides a solid refactoring of the delay system, moving from a cache-level TTL to a per-entry TTL managed by Instant. This is a great improvement for correctness and prevents premature evictions. The introduction of DefaultDelay and ExplicitDelay interfaces, along with a factory class, results in a much cleaner and more robust API. The implementation is well-structured, especially the shared logic in GuavaDelay. I've identified a few minor inconsistencies in handling expiration edge cases that could be addressed to further improve the robustness of the new system. Overall, this is a high-quality contribution.
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDefaultDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDelay.java
Outdated
Show resolved
Hide resolved
…Delay.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…Delay.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
eternalcore-api/src/main/java/com/eternalcode/core/delay/Delay.java
Outdated
Show resolved
Hide resolved
….java Co-authored-by: Igor Michalski <imichalsky00@gmail.com>
Jakubk15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me. Shout-out for the good documentation of the API.
| * | ||
| * @param <T> key type | ||
| */ | ||
| public interface DefaultDelay<T> extends ExplicitDelay<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to FixedDelay. Sounds better imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Powiem tak, jak dla mnie główny problem z issue nie został rozwiązany - expire w zależności od nadanego delay.
Też dostaliśmy dużą warstwę niepotrzebnej abstrakcji - kodzik jest bardziej skomplikowany, z jednej klasy zrobiło się parę plików i masa komentarzy.
Proponuję użyć caffeine do cache bo tam da się faktycznie dynamicznie operować tym czasem, obecnie rozwiązanie dużo się nie różni jakby zrobić HashMap z sprawdzaniem czy size nie został przekroczony - mogę wrzucić przykładową implementację (z caffeine)
| this.randomTeleportTaskService = randomTeleportTaskService; | ||
| this.randomTeleportSettings = randomTeleportSettings; | ||
| this.cooldown = Delay.withDefault(this.randomTeleportSettings.cooldown()); | ||
| this.cooldown = Delay.withDefault(() -> this.randomTeleportSettings.cooldown()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tutaj musi być provider () -> bo pobieramy wartość z configu a ona się reloaduje czasem
// kontrakt
public interface Delay<TYPE> {
void mark(TYPE key, Function<TYPE, Duration> durationProviderOverride);
void unmark(TYPE key);
boolean isActive(TYPE key);
Duration getRemaining(TYPE key);
Instant getExpireAt(TYPE key);
void extend(TYPE key, Duration extra);
static <TYPE> Delay<TYPE> ofDefault(Duration defaultDuration, long maximumSize) {
return new KeyedDelay<>(key -> defaultDuration, maximumSize);
}
static <TYPE> Delay<TYPE> ofDynamic(Function<TYPE, Duration> durationProvider, long maximumSize) {
return new KeyedDelay<>(durationProvider, maximumSize);
}
}
final class KeyedDelay<TYPE> implements Delay<TYPE> {
private final Function<TYPE, Duration> durationProvider;
private final Cache<TYPE, Instant> cache;
KeyedDelay(Function<TYPE, Duration> durationProvider, long maximumSize) {
this.durationProvider = durationProvider;
this.cache = Caffeine.newBuilder()
.maximumSize(maximumSize)
.build();
}
@Override
public void mark(TYPE key, Function<TYPE, Duration> durationProviderOverride) {
Function<TYPE, Duration> functor = durationProviderOverride != null ? durationProviderOverride : durationProvider;
Duration duration = functor.apply(key);
cache.put(key, Instant.now().plus(duration));
}
@Override
public void unmark(TYPE key) {
cache.invalidate(key);
}
@Override
public boolean isActive(TYPE key) {
Instant expire = cache.getIfPresent(key);
return expire != null && Instant.now().isBefore(expire);
}
@Override
public Duration getRemaining(TYPE key) {
Instant expire = cache.getIfPresent(key);
if (expire == null) {
return Duration.ZERO;
}
Duration remaining = Duration.between(Instant.now(), expire);
return remaining.isNegative() ? Duration.ZERO : remaining;
}
@Override
public Instant getExpireAt(TYPE key) {
return cache.getIfPresent(key);
}
@Override
public void extend(TYPE key, Duration extra) {
Instant expire = cache.getIfPresent(key);
Instant newExpire = (expire != null ? expire : Instant.now()).plus(extra);
cache.put(key, newExpire);
}
}uzycie: final class DelayExample {
void main(String[] args) throws InterruptedException {
// Tworzymy Delay z domyślnym czasem np. 5 sekund
Delay<String> delay = Delay.ofDefault(Duration.ofSeconds(5), 100);
String key = "hujsonpool";
// Sprawdzamy, czy mamy delay
System.out.println("Czy jest delay? " + delay.has(key)); // :: false
// Oznaczamy delay
delay.mark(key, null);
System.out.println("Ustawiono delay dla " + key);
// Pozostały czas
System.out.println("Pozostały czas: " + delay.getRemaining(key).toSeconds() + " sekund");
// Przykry przykład Czekamy 3 sekundy
Thread.sleep(3000);
System.out.println("Czy jest delay po 3 sekundach? " + delay.has(key));
System.out.println("Pozostały czas: " + delay.getRemaining(key).toSeconds() + " sekund");
// Przedłużamy o 2 sekundy
delay.extend(key, Duration.ofSeconds(2));
System.out.println("Przedłużono delay. Pozostały czas: " + delay.getRemaining(key).toSeconds() + " sekund");
// Czekamy kolejne 5 sekund
Thread.sleep(5000);
System.out.println("Czy jest delay po 8 sekundach? " + delay.has(key)); // :: false
System.out.println("Pozostały czas: " + delay.getRemaining(key).toSeconds() + " sekund");
// Usuwamy delay
delay.unmark(key);
System.out.println("Delay usunięty" + delay.has(key));
}
} |
vLuckyyy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-Request review from me, when you follow other's suggestion.
| * Provides per-entry delay management using Caffeine with wall-clock based expiration. | ||
| * | ||
| * @param <T> the key type | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wywal docsy
|
|
||
| this.defaultDelay = defaultDelay; | ||
| this.cache = Caffeine.newBuilder() | ||
| .maximumSize(maximumSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
po co nam maximumSize?
P1otrulla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple and small! Good job! 😎
noyzys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Panowie testy przeszły więc jest dobrze 🤪
Description
This PR refactors the delay handling to eliminate cache-level TTL conflicts.
expireAfterWriteto ensure TTL is controlled per entry via InstantResult: cleaner API, safer usage, no premature cache evictions.
Fixes #1153
Type of change
How Has This Been Tested?